Improve E2E setup #803

yannikmesserli · 2024-02-25T21:38:09Z

No issue

Description

This PR aims to simplify our E2E setup, so that it is easier for anyone to understand it (and hopefully faster for them to write tests) and easier to maintain.

Changes

Moved all test selectors and attributes to an enum at top of component: this will prevent building "legacy". Components are responsible for maintaining their test ids and if the component disapears, so do the test ids related, breaking the E2E scenarios using it.
Removed src/testing/e2e/cypress/client which were the main source of legacy building
Added a utils for the E2e tests: getTestAttribute, which helps adding the common data-test (and avoid attributes to be shown in production)
Moved up all cypress's folder
Documented the various Cypress command

After this PR, E2E test are written as follow:

Add an enum E2E on top of the component you'd like to interact with:

export enum E2E {
   myTestId = "test-id",
}

Attach the test attribute(s) using getTestAttributes as follow in your component:

<div {...getTestAttributes(E2E.myTestId)} />

or:

<div {...getTestAttributes({ [E2E.myTestAttr]: "some value" })} />

Use Cypress's new command query or queryAttrs to get the component in your scenario, importing the E2E test id within your test:

import { E2E } from '~/components/my-component';
// ...
cy.query(E2E.myTestId);
// or
cy.queryAttrs({ [E2E.myTestAttr]: "some value" });

Screenshots

Doc for queryAttrs in VSCode	Doc for query in VSCode

Unit tests

Not added. We could add some for getTestAttributes...

Functional tests

The e2e tests should still run and pass

Future work

We need to document the setup for e2e and how to write e2e test scenarios
The repo https://bitbucket.org/yandzee/hubble-ui-testing.git with test data needs to somehow be brought back in this repo (or at least in a cilium repo)

yandzee

I like changes regarding doc lines and webpack configuration in cypress setup, but that it because all other changes are making things worse.

We already had a conversation about this kind of changes, so I don't really understand why you still want to have them. I cannot approve this PR since such changes merely violates many simple architecture principles.

You suggest to use stateless getTestAttributes function right in UI components as a replacement for method call of the entity who is responsible to control the attributes for any logical part of component. This can be considered as a step backward from abstracting over that entity to concrete implementation.
With that you have to pass a special identifier into that function guiding e2e code from this end. This is kind a similar violation as what Cypress docs refer to when saying you not to rely to native html/css of components so that you could establish proper elements querying: the generalization of this principle could sound like - its not the component who should decide what attributes should be put for e2e tests. I would say this is also a step backward to depending on implementation rather then abstraction
As a consequences of this changes now you suggest to make e2e module dependable on the UI components itself - you must import those defined attributes from somewhere. This is also not correct: e2e module should define what they should use for tests, not vice versa.
A classical indicator of such architecture violation is that since you no more depend on abstraction/higher level responsible entity you have to customize all other details around used hardcoded implementation - this is what we see here in AccessPoint and ServiceMapArrowDuckFeet components - they are starting to define its own helpers with strong coupling on tests details. You will have to define its own helpers in each component in such an architecture.

src/components/AccessPoint/index.tsx

src/components/ServiceMapArrowRenderer/ServiceMapArrowDuckFeet.tsx

src/environment/index.ts

src/utils/test.ts

src/testing/e2e/support/index.ts

yandzee · 2024-02-26T09:03:50Z

src/testing/e2e/helpers/service-map.ts

+
+import { Preset } from './visit';
+import { welcomeScreen } from './welcome-screen';
+import { E2E as WelcomeScreenE2E } from '~/components/ServiceMapApp/WelcomeScreen';


You are going to depend on each module/component that is being tested, right?

Exactly the goal of this PR.

yandzee · 2024-02-26T09:05:00Z

cypress.config.ts


 export const webpackConfiguration = async (f: Cypress.FileObject) => {
-  const opts = { webpackOptions: { ...mainWebpackConfig, plugins: [] } };
+  // HtmlWebpackPlugin is clashing with cypress own html file process
+  const plugins = mainWebpackConfig.plugins.filter(f => !(f instanceof HtmlWebpackPlugin));


yannikmesserli · 2024-02-26T10:46:41Z

Summary of discussion with @yandzee on 26 Feb 2024:

We couldn't agree on the architecture (yet)
@yandzee wants the frontend to not know anything about the tests. Instead, creating the logic for the various keys inside the test folder, and the components to call them.
@yannikmesserli wants the frontend to have the test keys within the component and the tests importing them, so that we don't have to maintain logic for tests, simplify the setup.
We decided that both will seek examples of cypress setup in other projects.

Feel free to chip in to help us decide which route we should go.

yandzee · 2024-02-26T10:55:44Z

That's true in general, minor comments:

yandzee wants the frontend to not know anything about the tests. Instead, creating the logic for the various keys inside the test folder, and the components to call them.

Sure its better to minimize the amount of test logic you blend into UI logic/components. I don't want to have keys, I want to have abstract methods working in the way like "hey, here is the FlowsTable column, you may put some attributes for it if you want". In contrast what Yannik suggestion: "hey, I am the FlowsTable and its me who decides what attributes I am going to have, where and when"

yannikmesserli · 2024-02-28T10:21:00Z

This has been rejected by @yandzee and we could not find consensus within our team of developer. I am therefore closing it.

rolinh · 2024-03-04T09:51:48Z

We discussed this PR internally and agreed that this patch successfully addresses the goals of simplifying our e2e test setup and will make them easier to maintain in the long run. Although we may not all agree on some specifics of this PR, we agree that it is a net improvement overall and we should thus work on getting it merged.

yannikmesserli requested a review from a team as a code owner February 25, 2024 21:38

yannikmesserli requested review from yandzee and removed request for a team February 25, 2024 21:38

yannikmesserli force-pushed the pr/yannik/e2e-setup branch 4 times, most recently from 474956e to 9a904ee Compare February 26, 2024 09:04

Improve E2E setup

43c7db4

yannikmesserli force-pushed the pr/yannik/e2e-setup branch from 9a904ee to 43c7db4 Compare February 26, 2024 09:10

yandzee requested changes Feb 26, 2024

View reviewed changes

Remove any

d9fcb7f

yannikmesserli added 3 commits February 26, 2024 14:04

Move utils/test to testing/helpers

53c2b70

Avoid messing with application's classes

ae09ccb

Remove unnecessary function for testing keys

df3e4c1

yannikmesserli closed this Feb 28, 2024

rolinh reopened this Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve E2E setup #803

Improve E2E setup #803

yannikmesserli commented Feb 25, 2024

yandzee left a comment

yandzee Feb 26, 2024

yannikmesserli Feb 26, 2024

yandzee Feb 26, 2024

yannikmesserli commented Feb 26, 2024

yandzee commented Feb 26, 2024

yannikmesserli commented Feb 28, 2024

rolinh commented Mar 4, 2024

Improve E2E setup #803

Are you sure you want to change the base?

Improve E2E setup #803

Conversation

yannikmesserli commented Feb 25, 2024

Description

Changes

Screenshots

Unit tests

Functional tests

Future work

yandzee left a comment

Choose a reason for hiding this comment

yandzee Feb 26, 2024

Choose a reason for hiding this comment

yannikmesserli Feb 26, 2024

Choose a reason for hiding this comment

yandzee Feb 26, 2024

Choose a reason for hiding this comment

yannikmesserli commented Feb 26, 2024

yandzee commented Feb 26, 2024

yannikmesserli commented Feb 28, 2024

rolinh commented Mar 4, 2024